RDKEMW-24199: Fixed GetKnownSSIDs for expected response - #348
Conversation
Reason for change: GetKnownSSIDs must return stored WiFi Profile information. Priority: P2 Test Procedure: Refer ticket Risks: Low Signed-off-by: Mehavarshni_Palaniswamy@comcast.com
Reason for change: GetKnownSSIDs must return stored WiFi Profile information. Priority: P2 Test Procedure: Refer ticket Risks: Low Signed-off-by: Mehavarshni_Palaniswamy@comcast.com
Release of 4.0.0 Rebase
Reason for change: GetKnownSSIDs must return stored WiFi Profile information. Priority: P2 Test Procedure: Refer ticket Risks: Low Signed-off-by: Mehavarshni_Palaniswamy@comcast.com
There was a problem hiding this comment.
🟡 Changes recommended
The current handling of “no known SSIDs” can incorrectly return a list containing an empty string ([""]) instead of an empty list ([]), which is a behavioral bug for API consumers.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the NetworkManager plugin’s “known SSIDs” retrieval to return saved WiFi profile information even when the device is disconnected (notably on RDKV), by switching the RDK backend to query the paired/saved SSID rather than the currently connected SSID.
Changes:
- Add an IARM API identifier for retrieving the saved/paired SSID on RDK (
getPairedSSID). - Update the RDK implementation of
GetKnownSSIDsto callgetPairedSSIDinstead ofgetConnectedSSID. - Adjust the GNOME implementation of
GetKnownSSIDsto always build an iterator (even when no SSIDs are found).
File summaries
| File | Description |
|---|---|
| plugin/rdk/NetworkManagerRDKProxy.h | Adds the new IARM API string constant for getPairedSSID. |
| plugin/rdk/NetworkManagerRDKProxy.cpp | Switches GetKnownSSIDs to use the paired/saved SSID IARM call. |
| plugin/gnome/NetworkManagerGnomeProxy.cpp | Refactors GetKnownSSIDs iterator creation flow when the known-SSID list is empty. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Reason for change: GetKnownSSIDs must return stored WiFi Profile information. Priority: P2 Test Procedure: Refer ticket Risks: Low Signed-off-by: Mehavarshni_Palaniswamy@comcast.com
There was a problem hiding this comment.
🔵 Needs a closer look
Both RDK and GNOME paths can return a single empty-string SSID ([""]) instead of an empty list when no SSID is available, which is incorrect output for an SSID list.
Review details
Suppressed comments (2)
plugin/rdk/NetworkManagerRDKProxy.cpp:1051
- GetKnownSSIDs currently unconditionally pushes the returned
pairedSsid.ssidintossidList. If the IARM call succeeds but there is no paired SSID (empty string), this will serialize as[""]instead of an empty list, which is not a valid SSID value and can confuse clients.
auto &pairedSsid = param.data.getPairedSSID;
std::list<string> ssidList;
ssidList.push_back(string(pairedSsid.ssid));
plugin/gnome/NetworkManagerGnomeProxy.cpp:773
- When
ssidListis empty, this code inserts an empty string element. That will make the RPC/JSON response contain[""]instead of an empty array, which is semantically different and treats "no SSIDs" as a single SSID with an empty name.
if (ssidList.empty())
{
NMLOG_INFO("known ssids not found !");
ssidList.push_back(string());
}
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
Reason for change: Remove explicit rescan request upon wake-up(NSM-ON). Test Procedure: Change the channel in AP or change the timeout for SAE GTK_REKEY when the box is in DeepSleep & wake-up; ensure WiFi reconnected with 10s; wake-up journey. Priority: P1 Risks: Low Signed-off-by: Devika Jaladi <djalad690@cable.comcast.com> Co-authored-by: Karunakaran A <karunakaran_amirthalingam@cable.comcast.com>
Release of 4.1.0
…3s to 1s (#345) * RDKEMW-23908: Reduce default ping count (3->1), timeout (3s->1s), interval (0.2s->0.002s) Reason for change: Current Ping method is synchronous and blocks other method calls while ping is executing. This fix is a mitigation. A proper fix requires methods like Ping that trigger long-running operations to be asynchronous. Test Procedure: Refer ticket Risks: Low Signed-off-by: Tony_Ukken2@comcast.com Co-authored-by: Karunakaran A <48997923+karuna2git@users.noreply.github.com> Co-authored-by: Karunakaran A <karunakaran_amirthalingam@cable.comcast.com>
Release of 4.2.0
Reason for change: GetKnownSSIDs must return stored WiFi Profile information. Priority: P2 Test Procedure: Refer ticket Risks: Low Signed-off-by: Mehavarshni_Palaniswamy@comcast.com
Reason for change: GetKnownSSIDs must return stored WiFi Profile information. Priority: P2 Test Procedure: Refer ticket Risks: Low Signed-off-by: Mehavarshni_Palaniswamy@comcast.com
Reason for change: GetKnownSSIDs must return stored WiFi Profile information. Priority: P2 Test Procedure: Refer ticket Risks: Low Signed-off-by: Mehavarshni_Palaniswamy@comcast.com
Reason for change: GetKnownSSIDs must return stored WiFi Profile information. Priority: P2 Test Procedure: Refer ticket Risks: Low Signed-off-by: Mehavarshni_Palaniswamy@comcast.com
…networkmanager into feature/RDKEMW-24199
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved RDK reconnect, Ping interval, empty-list handling, and IARM status issues remain.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
plugin/NetworkManagerImplementation.cpp:559
-i 0.002requests a 2 ms interval. For counts greater than one this can burst up to 500 packets per second, and standard iputils rejects intervals below 200 ms for unprivileged callers, so Ping can either fail or create an avoidable CPU/network burst. Keep a supported interval or explicitly cap/validate the request and privilege requirements before using such a value.
snprintf(cmd, sizeof(cmd), "ping6 -c %d -W %d -i 0.002 '%s' 2>&1", noOfRequest, timeOutInSeconds, endpoint.c_str());
}
else
{
snprintf(cmd, sizeof(cmd), "ping -c %d -W %d -i 0.002 '%s' 2>&1", noOfRequest, timeOutInSeconds, endpoint.c_str());
plugin/NetworkManagerJsonRpc.cpp:585
- The title and description only cover the RDK
GetKnownSSIDsfix, but this hunk changes Ping defaults; the same diff also changes the ping interval and wake-up scan behavior. These are independent runtime changes not represented in the stated test procedure or low-risk assessment. Please split them into scoped changes or update the description, risk, and tests to cover them.
uint32_t noOfRequest = 1;
uint16_t timeOutInSeconds = 1;
plugin/gnome/NetworkManagerGnomeProxy.cpp:773
- The newly added empty-list branch does not handle the no-profile case with the current backend:
wifiManager::getKnownSSIDsreturnsfalsewhenssidsis empty (NetworkManagerGnomeWIFI.cpp:1850-1858), so execution never reaches this block and the API still returnssuccess:false. Propagate an empty list as a successful result from the backend, and keep it empty rather than adding a non-existent SSID.
if (ssidList.empty())
{
NMLOG_INFO("known ssids not found !");
ssidList.push_back(string());
}
- Files reviewed: 11/11 changed files
- Comments generated: 2
- Review effort level: Lite
| NMLOG_INFO("OnPowerModeChanged: waking from DeepSleep, requesting DHCP lease on wlan0"); | ||
| if (ReacquireDHCPLease("wlan0") != Core::ERROR_NONE) | ||
| { |
| /* Must add new method to get all the known SSIDs but for now RDK-NM supports only one saved SSID. */ | ||
| retVal = IARM_Bus_Call(IARM_BUS_NM_SRV_MGR_NAME, IARM_BUS_WIFI_MGR_API_getPairedSSID, (void *)¶m, sizeof(param)); | ||
|
|
||
| if(retVal == IARM_RESULT_SUCCESS) |
Reason for change: GetKnownSSIDs must return stored WiFi Profile information.
Priority: P2
Test Procedure: Refer ticket
Risks: Low
Signed-off-by: Mehavarshni_Palaniswamy@comcast.com